Fix case sensitive environment variable lookup#1001
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR modifies the ChangesCase-insensitive environment variable resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud_governance/main/environment_variables.py`:
- Line 395: The line assigning env_value currently treats empty strings as falsy
and can skip an explicitly set empty env var; update the lookup in the scope
where env_value is assigned (the expression using os.environ.get(var) /
var.upper() / var.lower() in environment_variables.py) to perform explicit
existence checks (e.g., check "if var in os.environ" then use os.environ[var],
else check var.upper(), var.lower(), else use defval) so that an explicitly set
empty string is preserved while still providing case-insensitive fallbacks and
finally the default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: fcc46fe7-4cf4-44aa-8760-253003f9ed53
📒 Files selected for processing (1)
cloud_governance/main/environment_variables.py
| def get_env(var: str, defval=''): | ||
| lcvar = var.lower() | ||
| dashvar = lcvar.replace('_', '-') | ||
| env_value = os.environ.get(var) or os.environ.get(var.upper()) or os.environ.get(var.lower()) or defval |
There was a problem hiding this comment.
Empty string handling bug in case-insensitive lookup.
The or chaining treats empty strings as falsy, which means an explicitly set environment variable like ES_PORT='' would be skipped in favor of checking other case variations or the default value. While unlikely in practice, this could cause confusing behavior where an explicitly set value is ignored.
🔧 Proposed fix using explicit existence checks
- env_value = os.environ.get(var) or os.environ.get(var.upper()) or os.environ.get(var.lower()) or defval
+ for key in (var, var.upper(), var.lower()):
+ if key in os.environ:
+ env_value = os.environ[key]
+ break
+ else:
+ env_value = defvalThis approach respects explicitly set empty strings while still providing the case-insensitive fallback behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env_value = os.environ.get(var) or os.environ.get(var.upper()) or os.environ.get(var.lower()) or defval | |
| for key in (var, var.upper(), var.lower()): | |
| if key in os.environ: | |
| env_value = os.environ[key] | |
| break | |
| else: | |
| env_value = defval |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud_governance/main/environment_variables.py` at line 395, The line
assigning env_value currently treats empty strings as falsy and can skip an
explicitly set empty env var; update the lookup in the scope where env_value is
assigned (the expression using os.environ.get(var) / var.upper() / var.lower()
in environment_variables.py) to perform explicit existence checks (e.g., check
"if var in os.environ" then use os.environ[var], else check var.upper(),
var.lower(), else use defval) so that an explicitly set empty string is
preserved while still providing case-insensitive fallbacks and finally the
default.
github-project-automation
Bot
moved this from In progress
to Done
in Cloud-Governance project
2 participants
Type of change
Note: Fill x in []
Description
The get_env() method was case-sensitive when looking up environment variables. Jenkins sets:
ES_HOST (uppercase)
ES_PORT (uppercase)
But the code was looking for:
es_host (lowercase)
es_port (lowercase)
On Linux/Unix systems, os.environ is case-sensitive, so os.environ.get('es_port') won't find ES_PORT, returning an empty string ''. Then when the code tried to do int('') on line 53 or 63 of elasticsearch_operations.py, it failed with invalid literal for int() with base 10: ''.
Fix
Updated the get_env() method in environment_variables.py to check for the variable in multiple case variations
For security reasons, all pull requests need to be approved first before running any automated CI
Assisted-by: Cursor